[QHC-833] Improving digital Transpilation #862

GuillermoAbadLopez · 2024-12-23T23:49:06Z

TODOs for this PR:

(each of the next points is pretty big deal, so check them all please 🙌🙃)

Move optimize flag, for actual optional optimizations (& Improve optimize word use in methods names)
Make Transpilation/execute/compile only for single circuits (unify code standard across qililab)
Make Transpilation efficient, by not constructing the Circuit class so many times, between methods
Pass a transpilation kwargs instead of so many args in platform/qililab's execute(...)
Improve documentation on transpilation, simplifying it in execute()'s, and moving it to a new section.
Cover/change test for all of this.

You will see two empty methods, in this next PR I will fill them:

bunch_drag_gates(...), delete_gates_with_no_amplitude(...)

The documentation after this PR looks like:

For the `platform.execute()`:

For the module about `Transpilation`:

For the `Transpiler.transpile_circuit()`:

pullpo-for-slack · 2024-12-23T23:49:17Z

🔗Pullpo.io Slack channel

pullpo-for-slack · 2024-12-23T23:49:19Z

AI Analysis

Review:

The changes in circuit_transpiler.py to add phases from RZs and CZs gates to Drag gates, optimize transpilation by combining consecutive Drags, and adding explanatory comments seem well-structured and clear. It's great to see the addition of optimization methods for the transpilation process. In circuit_optimizer.py, the addition of the method to add phases to Drag gates and optimize transpilation by combining consecutive Drags aligns well with the changes made in circuit_transpiler.py. It's good to maintain consistency across the codebase.

Detailed file changes

(dropdown):

In circuit_transpiler.py:

Added a method to add phases from RZs and CZs gates to Drag gates in the circuit.
Modified the transpile_circuits method to use the new method to add phases to Drags.
Added comments explaining the process of adding phases from RZs and CZs gates to Drags.
Added a method to optimize transpilation by bunching consecutive Drag gates together into a single one.

In circuit_optimizer.py:

Added a method to add phases from RZs and CZs gates to Drag gates in the circuit.
Modified the cancel_pairs_of_hermitian_gates method signature to return a Circuit.
Added a method to optimize transpilation by bunching consecutive Drag gates together into a single one.

linear · 2024-12-28T16:22:01Z

QHC-833 Transpilation feedback/improvements

codecov · 2024-12-28T19:03:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.30%. Comparing base (7226edf) to head (2584953).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #862      +/-   ##
==========================================
+ Coverage   96.77%   97.30%   +0.53%     
==========================================
  Files         221      231      +10     
  Lines        7756     8283     +527     
==========================================
+ Hits         7506     8060     +554     
+ Misses        250      223      -27

Flag	Coverage Δ
unittests	`97.30% <100.00%> (+0.53%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…n compile and execute

fedonman

Nice job! Left some comments.

fedonman · 2025-01-08T14:51:35Z

src/qililab/execute_circuit.py

-    router: Router | type[Router] | tuple[type[Router], dict] | None = None,
-    routing_iterations: int = 10,
-    optimize: bool = True,
+    program: Circuit | list[Circuit], runcard: str | dict, nshots: int = 1, transpile_config: dict = {}


I don't like replacing the parameters with a generic dictionary. Now, it is not clear what the dictionary should contain, what are the default values, there is no autocomplete etc.

In general, having all options as separate parameters is preferred.

If we really do need to use a single parameter, we should use a NamedDict or a new class.

Also, default parameter values should not be mutable: https://docs.astral.sh/ruff/rules/mutable-argument-default/

Also, default parameter values should not be mutable: https://docs.astral.sh/ruff/rules/mutable-argument-default/

Damn, true... I guess I'll have to set it back to None then... started like that, but that made the code much uglier, with more checks, that complicated it... 😅. But definitely can go back to it, makes sense 👌

I don't like replacing the parameters with a generic dictionary. Now, it is not clear what the dictionary should contain, what are the default values, there is no autocomplete etc.

In general, having all options as separate parameters is preferred.

If we really do need to use a single parameter, we should use a NamedDict or a new class.

Yeah I didn't like it either, but it was so many parameters, which might keep growing..., all of them related to the same thing, that in the end discussing it with @visagim we thought, it didn't make sense to keep the params separated, mixing with the other compile/execute parameters, when they clearly are more "modular" 😓. (That's also why I only did it for compile/execute's, but not for the transpile methods down the line)

I also tried first to do a default class, but what I didn;t like about it, is the user having to import and initialize a random class, from a file in qililab, dunno how intuitive that would be 😅. But its true that at least you would then have initialization.

Dunno I'm not 100% convinced what is better, I can try to generate the three examples:

Keep params separated

Ganeric kwargs

Specific qililab dataclass

and ask again in a Code Discussion/daily, where everybody can see it and decide 👍 :

src/qililab/platform/platform.py

fedonman · 2025-01-08T15:00:39Z

src/qililab/digital/circuit_router.py

@@ -70,6 +70,7 @@ def __init__(
        # 3) Layout stage, where the initial_layout will be created.

    def route(self, circuit: Circuit, iterations: int = 10) -> tuple[Circuit, dict[str, int]]:
+        # Docstring related to the public method: :meth:`.CircuitTranspiler.route_circuit()`. Change it there too.


Is this comment needed here?

Yeah, good point... I actually have a problem here, which I wanted to ask about:

If you see, Inside the Transpiler we have basically methods duplicated from the Optimizer..., since we removed the handling of multiple circuits at the same time.. One option would be to remove such duplication, but at the same time that duplications serves as a kind of interface to add more functionality, and shows the user which functions can be called from the transpiler directly, so its not super bad...

Of course this makes that the docstrings are duplicated, and that is more problematic, since we wouldn't; want our devs, only updating the Optimizer docstrings, but not the Transpiler's which are more public. So the options we have are:

Delete the duplication in some way (I think this will complicate how we show public method to the user...)

Add this silly comment for devs to notice, that they need to change the public documentation always

Maybe just delete the docstring from the not public methods? So there is no mistake by the devs? And they know to go and search elsewhere, which docstring to change 🤔...

I'll think more about it the next days, and ask the group with a clearer head...

But such problem make me notice there is a structural code problem from base with having this duplication, which maybe should be addressed more deeply... But 😬 ...

@fedonman, I ended up deciding to delete a bunch of duplicated docstrings, so we don't have confusion on where to update them!

And deleted the comments you pointed out, instead now I added inside the duplicated docstring itself, check if you agree with the implementation 🙌

src/qililab/digital/circuit_to_pulses.py

src/qililab/digital/circuit_transpiler.py

docs/fundamentals/transpilation.rst

Improving optimization word use in the transpilation

a0f70db

GuillermoAbadLopez self-assigned this Dec 23, 2024

GuillermoAbadLopez changed the title ~~Improving optimization word use in the transpilation~~ [QHC-833] Improving optimization word use in the transpilation Dec 28, 2024

GuillermoAbadLopez changed the title ~~[QHC-833] Improving optimization word use in the transpilation~~ [QHC-833] Improving Transpilation Dec 28, 2024

GuillermoAbadLopez changed the title ~~[QHC-833] Improving Transpilation~~ [QHC-833] Improving/Feedback Transpilation Dec 28, 2024

GuillermoAbadLopez changed the title ~~[QHC-833] Improving/Feedback Transpilation~~ [QHC-833] Improving/Feedback in Transpilation Dec 28, 2024

GuillermoAbadLopez changed the title ~~[QHC-833] Improving/Feedback in Transpilation~~ [QHC-833] Improving/Feedback for Transpilation Dec 28, 2024

GuillermoAbadLopez added 2 commits December 28, 2024 20:00

Refactor/improve digital Transpiler

0288e5a

Merge branch 'main' into solving-transpilation

5313770

GuillermoAbadLopez requested review from fedonman and visagim December 28, 2024 19:08

make optimize default to False

354f58e

GuillermoAbadLopez removed request for fedonman and visagim December 28, 2024 19:16

GuillermoAbadLopez added 12 commits December 28, 2024 20:50

Correcting docstrings and adding transpilation parameters as kwargs i…

ab6b082

…n compile and execute

make transpilation_config shorter

48a3e9e

Make empty dict default

7b8dd49

Update docstrings

90bb445

Solving docstring problems

217740a

Solving problems documentation

8f14eae

Improving documentation

c3703d6

improving documentation

8989484

Update test_circuit_transpiler.py

b6770ec

Improve documentation transpiler

7506b2d

Improving documentation transpiler

27e4874

Solving documentation transpilation

def668f

First try of Transpilation summary

cc56d71

GuillermoAbadLopez requested review from fedonman and visagim December 29, 2024 01:03

GuillermoAbadLopez added 8 commits December 30, 2024 03:03

Update circuit_transpiler.py

72c9c5d

Update test_circuit_transpiler.py

c22ac09

Improving documentation for transpilation

7d0f697

Improving linking between transpilaiton documentation

0764d9b

Merge branch 'main' into solving-transpilation

44cc7ec

Update platform.py

51fab0c

Improve documentation further

872c2fe

Adding comments for development

9c17cbd

GuillermoAbadLopez changed the title ~~[QHC-833] Improving/Feedback for Transpilation~~ [QHC-833] Improving digital Transpilation Dec 30, 2024

GuillermoAbadLopez added 2 commits December 31, 2024 13:38

Update circuit_transpiler.py

bddc5a8

Update test_platform.py

7fb7339

fedonman requested changes Jan 8, 2025

View reviewed changes

GuillermoAbadLopez added 3 commits January 8, 2025 17:38

Update circuit_transpiler.py

8b23588

Deleting duplicated docstrings

a5733c7

Make transpile-config kwargs, default value not mutable

2584953

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QHC-833] Improving digital Transpilation #862

[QHC-833] Improving digital Transpilation #862

GuillermoAbadLopez commented Dec 23, 2024 •

edited

Loading

pullpo-for-slack bot commented Dec 23, 2024

pullpo-for-slack bot commented Dec 23, 2024 •

edited

Loading

Detailed file changes

linear bot commented Dec 28, 2024

codecov bot commented Dec 28, 2024 •

edited

Loading

fedonman left a comment

fedonman Jan 8, 2025

fedonman Jan 8, 2025

GuillermoAbadLopez Jan 8, 2025

GuillermoAbadLopez Jan 8, 2025 •

edited

Loading

fedonman Jan 8, 2025

GuillermoAbadLopez Jan 8, 2025

GuillermoAbadLopez Jan 8, 2025

GuillermoAbadLopez Jan 13, 2025 •

edited

Loading

[QHC-833] Improving digital Transpilation #862

Are you sure you want to change the base?

[QHC-833] Improving digital Transpilation #862

Conversation

GuillermoAbadLopez commented Dec 23, 2024 • edited Loading

TODOs for this PR:

You will see two empty methods, in this next PR I will fill them:

The documentation after this PR looks like:

For the platform.execute():

For the module about Transpilation:

For the Transpiler.transpile_circuit():

pullpo-for-slack bot commented Dec 23, 2024

pullpo-for-slack bot commented Dec 23, 2024 • edited Loading

AI Analysis

Review:

Detailed file changes

linear bot commented Dec 28, 2024

codecov bot commented Dec 28, 2024 • edited Loading

Codecov Report

fedonman left a comment

Choose a reason for hiding this comment

fedonman Jan 8, 2025

Choose a reason for hiding this comment

fedonman Jan 8, 2025

Choose a reason for hiding this comment

GuillermoAbadLopez Jan 8, 2025

Choose a reason for hiding this comment

GuillermoAbadLopez Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

fedonman Jan 8, 2025

Choose a reason for hiding this comment

GuillermoAbadLopez Jan 8, 2025

Choose a reason for hiding this comment

GuillermoAbadLopez Jan 8, 2025

Choose a reason for hiding this comment

GuillermoAbadLopez Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

GuillermoAbadLopez commented Dec 23, 2024 •

edited

Loading

For the `platform.execute()`:

For the module about `Transpilation`:

For the `Transpiler.transpile_circuit()`:

pullpo-for-slack bot commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

GuillermoAbadLopez Jan 8, 2025 •

edited

Loading

GuillermoAbadLopez Jan 13, 2025 •

edited

Loading